feat: allow per-PR fix-loop budget via label - #1042
Conversation
Functional tests did not runFunctional tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the |
PR Summary by QodoAllow per-PR fix-loop budgets via labels
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. PR_LABELS is never populated
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| # A per-PR `fullsend-fix-budget/N` label may tighten the cap (never raise it). | ||
| FIX_BUDGET="$(parse_fix_budget "${PR_LABELS:-}")" | ||
| if [[ -n "${FIX_BUDGET}" && "${FIX_BUDGET}" -lt "${CAP}" ]]; then |
There was a problem hiding this comment.
1. pr_labels is never populated 📜 Skill insight ≡ Correctness
The new cap logic in pre-fix reads PR_LABELS, but neither the checked-in workflow nor the fix harness supplies PR labels through that environment variable, so real runs default to an empty value. Consequently, the advertised fullsend-fix-budget/N label cannot affect normal runtime execution until the missing wiring is added, as the PR description itself acknowledges.
Agent Prompt
## Issue description
The new fix-budget parser is invoked with `PR_LABELS`, but the runtime path never populates that variable, so `fullsend-fix-budget/N` labels cannot affect the fix cap.
## Issue Context
Fetch the current PR/MR's authoritative label names in the workflow and harness dispatch path, covering both supported forges, and pass them as the newline-separated `PR_LABELS` runner environment value expected by `pre-fix`. Preserve the existing empty-value behavior when labels cannot be obtained.
## Fix Focus Areas
- scripts/pre-fix.src.sh[114-119]
- harness/fix.yaml[65-91]
- .github/workflows/fullsend.yaml[22-59]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| FIX_BUDGET="$(parse_fix_budget "${PR_LABELS:-}")" | ||
| if [[ -n "${FIX_BUDGET}" && "${FIX_BUDGET}" -lt "${CAP}" ]]; then | ||
| gha_echo notice "PR label ${FIX_BUDGET_LABEL_PREFIX}${FIX_BUDGET} tightens the fix cap from ${CAP} to ${FIX_BUDGET}." | ||
| CAP="${FIX_BUDGET}" | ||
| fi |
There was a problem hiding this comment.
3. Cap enforcement remains untested 📜 Skill insight ▣ Testability
The added tests exercise only parse_fix_budget; none runs pre-fix to verify that a lower label changes CAP and stops an iteration above the tightened cap. The production behavioral change therefore lacks a corresponding test.
Agent Prompt
## Issue description
Parser unit tests do not cover the production enforcement path added to `pre-fix`.
## Issue Context
Add tests that execute the pre-fix path with bot and human caps, verify a lower valid label tightens the selected cap, verify a higher label cannot raise it, and verify an iteration above the tightened cap exits through escalation.
## Fix Focus Areas
- scripts/pre-fix.src.sh[114-121]
- scripts/pre-fix-test.sh[30-53]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # A per-PR `fullsend-fix-budget/N` label may tighten the cap (never raise it). | ||
| FIX_BUDGET="$(parse_fix_budget "${PR_LABELS:-}")" | ||
| if [[ -n "${FIX_BUDGET}" && "${FIX_BUDGET}" -lt "${CAP}" ]]; then | ||
| gha_echo notice "PR label ${FIX_BUDGET_LABEL_PREFIX}${FIX_BUDGET} tightens the fix cap from ${CAP} to ${FIX_BUDGET}." | ||
| CAP="${FIX_BUDGET}" |
There was a problem hiding this comment.
4. Fix-budget behavior is undocumented 📜 Skill insight ⚙ Maintainability
The repository documentation still describes only the global bot and human iteration caps and claims humans remain available through ITERATION_CAP_HUMAN, without documenting that a per-PR label can lower either selected cap. This makes the documented fix-loop semantics incomplete and potentially misleading.
Agent Prompt
## Issue description
The new public label and its effect on bot/human iteration caps are absent from repository documentation.
## Issue Context
Document `fullsend-fix-budget/N`, valid values, smallest-label behavior, tightening-only semantics, and how it interacts with both global caps.
## Fix Focus Areas
- agents/fix.md[174-187]
- scripts/pre-fix.src.sh[114-118]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| parse_fix_budget() { | ||
| local labels="${1-${PR_LABELS:-}}" | ||
| local best="" label n | ||
| while IFS= read -r label; do |
There was a problem hiding this comment.
5. Feature lacks linked authorization 📜 Skill insight § Compliance
This PR adds a new parser, runtime guard, generated bundle changes, and tests well beyond the rule's 20-line threshold, but the PR metadata contains no linked authorizing issue. The non-trivial feature therefore lacks the required explicit authorization.
Agent Prompt
## Issue description
The non-trivial feature change has no linked issue authorizing its scope.
## Issue Context
Link an issue that explicitly authorizes the per-PR fix-budget feature and confirms the intended producer wiring and enforcement scope.
## Fix Focus Areas
- scripts/lib/fix-budget.lib.sh[1-42]
- scripts/pre-fix.src.sh[114-119]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @@ -0,0 +1,42 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
6. Protected scripts require human approval 📜 Skill insight § Compliance
The PR modifies multiple files under the protected scripts/ path, so it must receive human review and must not be auto-approved. The feature rationale provides context, but there is no linked issue authorizing these governance/infrastructure changes.
Agent Prompt
## Issue description
This PR changes protected `scripts/` infrastructure and cannot be auto-approved.
## Issue Context
Route the PR for human approval and link the authorizing issue for the protected-path changes before merge.
## Fix Focus Areas
- scripts/lib/fix-budget.lib.sh[1-42]
- scripts/pre-fix.src.sh[20-29]
- scripts/pre-fix.src.sh[114-119]
- scripts/pre-fix-test.sh[1-59]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| [[ "${n}" =~ ^[1-9][0-9]*$ ]] || continue | ||
| if [[ -z "${best}" || "${n}" -lt "${best}" ]]; then | ||
| best="${n}" |
There was a problem hiding this comment.
7. Oversized budgets overflow arithmetic 🐞 Bug ≡ Correctness
The parser accepts an arbitrarily long digit string and compares it with Bash's bounded signed-integer arithmetic; for example, fullsend-fix-budget/18446744073709551616 evaluates as zero, is treated as tighter than cap 5, and then causes iteration 1 to exceed the effective cap. A syntactically valid oversized label can therefore block all fix runs instead of being ignored as a non-tightening budget.
Agent Prompt
## Issue description
Arbitrary-length decimal budgets are accepted and then evaluated with bounded Bash arithmetic, allowing overflow to turn a huge non-tightening value into an effective zero cap.
## Issue Context
Either reject values outside a documented safe integer range before any arithmetic operation, or compare normalized decimal strings by length and lexicographic order. Ensure selection of the smallest label and comparison with the configured cap both use the same overflow-safe validation/comparison, and add an oversized-label regression test.
## Fix Focus Areas
- scripts/lib/fix-budget.lib.sh[35-37]
- scripts/pre-fix.src.sh[115-121]
- scripts/pre-fix-test.sh[36-45]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| FIX_BUDGET="$(parse_fix_budget "${PR_LABELS:-}")" | ||
| if [[ -n "${FIX_BUDGET}" && "${FIX_BUDGET}" -lt "${CAP}" ]]; then | ||
| gha_echo notice "PR label ${FIX_BUDGET_LABEL_PREFIX}${FIX_BUDGET} tightens the fix cap from ${CAP} to ${FIX_BUDGET}." | ||
| CAP="${FIX_BUDGET}" |
There was a problem hiding this comment.
8. Post-fix ignores effective budget 🐞 Bug ◔ Observability
Only pre-fix computes the label-tightened cap, while post-fix still derives warnings and summaries from the global ITERATION_CAP. With budget 2 and global cap 5, the final allowed cycle reports 2 of 5 and does not add needs-human; the next cycle is simply rejected by pre-fix, so the existing escalation signal no longer matches enforcement.
Agent Prompt
## Issue description
The label-adjusted cap is enforced only in pre-fix, leaving post-fix's needs-human warning and iteration summary based on the old global cap.
## Issue Context
Compute the same effective cap in post-fix from the authoritative `PR_LABELS` value (preferably through a shared helper), then use it for warning thresholds and summaries. Add integration coverage showing a budget below the global cap produces the correct final-cycle warning and displayed cap.
## Fix Focus Areas
- scripts/pre-fix.src.sh[114-119]
- scripts/post-fix.src.sh[401-415]
- scripts/post-fix.src.sh[427-430]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
4399fec to
b35ed42
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass on the per-PR fix-loop budget. Four findings are posted inline; three below concern files this PR does not touch, so they have no anchorable diff line.
[HIGH] New fullsend-fix-budget/N control label missing from the post-review control-label denylist
scripts/post-review.src.sh:284
Verified at head. REVIEW_CONTROL_LABELS (post-review.src.sh:284-287) lists only ready-for-merge, requires-manual-review, rejected, ready-for-review, fullsend-no-fix, fullsend-fix, and is_control_label() (289-301) adds exactly one prefix check, risk/*. The new fullsend-fix-budget/N control label is covered by neither.
is_control_label gates the review agent's label_actions at post-review.src.sh:349-352 — the same guard that exists specifically to stop a prompt-injected review from attaching fullsend-no-fix. Two concrete attacks:
add— bounded by thelabel_existscheck at 355, so it only works once a maintainer has created afullsend-fix-budget/Nlabel; but that is precisely the population using this feature, and addingfullsend-fix-budget/1starves the fix loop.remove— the remove branch at 361-363 has nolabel_existsguard at all, so an injected review can strip a maintainer'sfullsend-fix-budget/2label and silently restore the loose global cap, defeating the control entirely.
This is an omission caused by the PR rather than a defect in a changed line, which is why it appears here rather than inline.
Suggestion. Add a fullsend-fix-budget/* prefix check to is_control_label() mirroring the existing risk/* check, and add post-review-test.sh cases asserting both add and remove of a fullsend-fix-budget/N label are refused. Regenerate scripts/post-review.sh with make script-build.
[MEDIUM] New user-facing control label missing from the docs/fix.md control-labels table
docs/fix.md:51
Verified at head. docs/fix.md:46-51 has a "Control labels" table that is the user-facing reference for this agent; it lists exactly two rows, fullsend-no-fix and needs-human, and the surrounding text at 42-44 documents only /fs-fix-stop. This PR adds a third maintainer-applied control label, fullsend-fix-budget/N, but documents it only in agents/fix.md — the prompt shipped into the sandbox, rather than where a maintainer looks. Nothing states that this label, unlike /fs-fix-stop, also blocks manual /fs-fix. A label nobody can discover cannot be used, which compounds the delimiter inertness flagged inline on harness/fix.yaml:71.
Partial overlap with the outdated bot comment on scripts/pre-fix.src.sh:118, whose fix focus named agents/fix.md only — that file is now updated, leaving docs/fix.md as the residual gap.
Suggestion. Add a row to the docs/fix.md "Control labels" table: fullsend-fix-budget/N — caps the review→fix loop at N iterations for this PR; can only lower the global cap, applies to bot and human runs (unlike fullsend-no-fix, which still permits manual /fs-fix), malformed values ignored, removing the label restores the global cap. Also mention it in the iteration-limits section around line 156.
[MEDIUM] No post-fix test that needs-human and the summary honour a tightened cap
scripts/post-fix-test.sh
Verified at head. fix-budget.lib.sh is newly sourced by post-fix.src.sh:69 specifically so post-fix can mirror the tightened cap into WARN_THRESHOLD (post-fix.src.sh:414-418) and the iteration summary (~line 440). But grepping scripts/post-fix-test.sh (1032 lines) for PR_LABELS, budget, or fix-budget yields zero hits — the file was not extended at all. The mirrored-budget path in post-fix is therefore exercised only by the parser unit tests in pre-fix-test.sh, never by an end-to-end assertion that the label changes post-fix behaviour.
Not a duplicate of the existing bot threads: one targeted missing pre-fix enforcement tests (now covered by pre-fix-test.sh:88-127) and another targeted post-fix ignoring the budget entirely (now fixed at head by the mirroring code); the untested post-fix path is what remains.
Related uncovered edge: a fullsend-fix-budget/1 label drives BOT_CAP=1 and WARN_THRESHOLD=0, so the [ "${ITERATION}" -ge "${WARN_THRESHOLD}" ] guard fires on iteration 1 and needs-human is applied after the very first fix run.
Suggestion. Add post-fix-test.sh cases: (a) bot run at iteration 2 with PR_LABELS=fullsend-fix-budget/2 and ITERATION_CAP=5 asserts needs-human is applied and the summary reports "2 of 2", not "2 of 5"; (b) a label value above the global cap has no effect; (c) budget=1 pins the intended iteration-1 needs-human behaviour. Consider clamping WARN_THRESHOLD to a minimum of 1 so the threshold cannot go non-positive if the digit-bound regex is ever loosened.
| # `fullsend-fix-budget/N` label tighten the iteration cap. The dispatcher | ||
| # (reusable-dispatch.yml, upstream fullsend) supplies the value; when it is | ||
| # absent this expands to empty and the cap is unchanged. | ||
| PR_LABELS: "${PR_LABELS}" |
There was a problem hiding this comment.
[CRITICAL] PR_LABELS in env.runner is fail-CLOSED: every fix run aborts at environment validation
Verified against primary sources in fullsend-ai/fullsend. The new comment claims "when it is absent this expands to empty and the cap is unchanged." The opposite is true.
internal/harness/harness.go:685-689documentsValidateRunnerEnvWithas: "Variables set to an empty string are allowed; only truly unset variables produce an error", and its loop overh.Env.Runnerreturnsenv.runner[%s]: host variable %s is not setwhenlookup()reports false.internal/cli/run.go:790callsh.ValidateRunnerEnvWith(lookup)insiderunAgentbefore anyos.Expand, andlookupisos.LookupEnv.TestValidateRunnerEnvWith_ChecksEnvRunner(harness_test.go:529) asserts exactly this for anenv.runnervalue of${MISSING_VAR}.action.yml:415invokesfullsend run "${AGENT}", i.e. that path.
PR_LABELS is genuinely unset there. Grepping the whole repo, PR_LABELS appears only at reusable-dispatch.yml:144 and internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml:43, both as step-scoped env on the "Determine stage" step of a different job. The "Run fix agent" step (reusable-fix.yml:359-376) has no PR_LABELS entry, and nothing writes it to GITHUB_ENV.
Every other key in this env.runner block is backed by a step-level env: entry or a GITHUB_ENV write (e.g. GIT_BOT_EMAIL at reusable-fix.yml:164), which GHA sets to empty-string rather than leaving unset — that is why the pattern has worked until now.
The new tests do not catch this: scripts/pre-fix-test.sh:88 sets PR_LABELS explicitly inside env -i, exercising only the script layer, never the harness validation layer. The PR body's "the feature is inert until wired" reasoning does not hold — the harness ships with this PR, so the failure fires on the next agents pin bump with zero workflow changes.
Note: the existing bot comment on scripts/pre-fix.src.sh:116 ("pr_labels is never populated") predates the current head and asserts the opposite failure mode — that runs merely default to empty and the feature is inert. The abort behavior is a distinct, unreported defect in a different file.
Suggestion. Drop the PR_LABELS entry from env.runner in this PR. Both scripts already read ${PR_LABELS:-} straight from the process environment (pre-fix.src.sh:115, post-fix.src.sh:414), so parsing/enforcement works without it. Add the env.runner line in the wiring PR, together with the reusable-fix.yml "Run fix agent" env: entry that guarantees the variable is set-possibly-empty. If it must land now, the workflow wiring has to land in the same change.
| TRIGGER_SOURCE: "${TRIGGER_SOURCE}" | ||
| HUMAN_INSTRUCTION: "${HUMAN_INSTRUCTION}" | ||
| FIX_ITERATION: "${FIX_ITERATION}" | ||
| # Newline-separated PR label names. Consumed by pre-fix/post-fix to let a |
There was a problem hiding this comment.
[MEDIUM] Comment states a dispatcher contract that does not exist, and the delimiter contradicts upstream
Verified against fullsend-ai/fullsend. This comment asserts as fact that "The dispatcher (reusable-dispatch.yml, upstream fullsend) supplies the value", while the PR body says the opposite ("a new optional input that the fix workflow does not populate yet").
reusable-dispatch.yml:144 does define PR_LABELS, but:
- It is step-scoped to "Determine stage" only, never reaching the fix agent step.
- It is comma-joined:
${{ join(github.event.pull_request.labels.*.name, ',') }}— matching the repo-wide convention, sincehas_label()atreusable-dispatch.yml:201-204doesIFS=',' read -ra labels.
parse_fix_budget (fix-budget.lib.sh:29,44) splits on newlines only, via while IFS= read -r label ... <<< "${labels}". If the follow-up wiring reuses the existing dispatcher value — the obvious move given the identical name — the parser silently never matches: bug,fullsend-fix-budget/2,area/api fails the prefix test at line 33, and fullsend-fix-budget/2,area/api fails the ^[1-9][0-9]{0,4}$ bound at line 40. The feature would be permanently inert with no error. GHA expressions also make joining on a literal newline awkward, so the wiring needs deliberate multiline construction rather than a one-line passthrough.
Suggestion. Correct the comment to state the value is not yet supplied. Then either accept the comma delimiter to match the upstream PR_LABELS convention (split on , as well as newline in parse_fix_budget, with comma cases added to pre-fix-test.sh), or rename the input (e.g. PR_LABELS_MULTILINE) so it cannot be confused with the comma-joined upstream variable.
| FIX_BUDGET="$(parse_fix_budget "${PR_LABELS:-}")" | ||
| if [[ -n "${FIX_BUDGET}" && "${FIX_BUDGET}" -lt "${CAP}" ]]; then | ||
| gha_echo notice "PR label ${FIX_BUDGET_LABEL_PREFIX}${FIX_BUDGET} tightens the fix cap from ${CAP} to ${FIX_BUDGET}." | ||
| CAP="${FIX_BUDGET}" |
There was a problem hiding this comment.
[MEDIUM] Bot escalation message reports the un-tightened human cap
Anchored at the budget block; the defective message is line 125 (bundled copy: scripts/pre-fix.sh:523).
HUMAN_CAP is assigned from ITERATION_CAP_HUMAN at line 107 and never tightened — the budget tightening at 114-118 writes only to CAP. The bot-branch escalation at line 125 prints:
A human can still direct the agent with /fs-fix (up to ${HUMAN_CAP} total iterations).
With a fullsend-fix-budget/2 label, a bot run at iteration 3 prints "up to 10 total iterations" while the very next human /fs-fix is rejected with "exceeds human cap of 2" — confirmed by this PR's own test at pre-fix-test.sh:127 (run_prefix "alice" 3 ITERATION_CAP_HUMAN 10 $'fullsend-fix-budget/2' expecting "exceeds human cap of 2"). The message actively misleads at the exact moment a maintainer needs accurate guidance.
Suggestion. Apply the budget to both BOT_CAP and HUMAN_CAP up front, before the bot/human branch selects CAP, so the escalation text and the enforced cap cannot diverge; and add "remove the fullsend-fix-budget/N label to lift this" to the message. Regenerate scripts/pre-fix.sh with make script-build and add a test asserting the bot-escalation message names the tightened human cap.
| (default: 10) total iterations (bot + human combined). This ensures humans | ||
| are never locked out of the agent after a bot loop exhausts its budget. | ||
|
|
||
| A maintainer can tighten the loop for a single PR with a |
There was a problem hiding this comment.
[MEDIUM] Label tightens the human cap, contradicting the "humans are never locked out" guarantee one paragraph above
Lines 183-187 state the design guarantee verbatim:
A human can then direct the agent with
/fs-fixcommands up toITERATION_CAP_HUMAN(default: 10) total iterations (bot + human combined). This ensures humans are never locked out of the agent after a bot loop exhausts its budget.
The paragraph added immediately after says the smallest valid label "lowers whichever cap applies (bot or human) to N", and pre-fix.src.sh:114-118 applies min(budget, CAP) after the bot/human branch, so a fullsend-fix-budget/2 label blocks human /fs-fix at iteration 3 too — asserted by this PR's own test at pre-fix-test.sh:127. The guarantee sentence is now false and is left standing unamended, so the two adjacent paragraphs contradict each other.
This partially overlaps the outdated bot comment on scripts/pre-fix.src.sh:118, which flagged the label as undocumented; at head agents/fix.md is updated, so that thread reads as addressed — the remaining defect is the self-contradiction, which was not reported.
Suggestion. Decide explicitly: either apply the budget to the bot cap only (preserving the invariant), or amend the preceding paragraph to state that a fullsend-fix-budget/N label is the one thing that can lock a human out, and include "remove the fullsend-fix-budget/N label to lift this" in the human-cap escalation message at pre-fix.src.sh:128.
Add a fullsend-fix-budget/N PR label that lets a maintainer cap the review->fix loop for a single PR below the global iteration cap. The label can only tighten the cap, never raise it: pre-fix applies the budget to both the bot and human caps before selecting one, so the human cap referenced in the bot-escalation message reflects the same effective budget. Parsing lives in a small, pure helper (scripts/lib/fix-budget.lib.sh) that is unit-tested directly (scripts/pre-fix-test.sh) and bundled into pre-fix/post-fix. It accepts both the upstream comma-joined label format and newline-joined input. Malformed label values are ignored rather than fatal, so a bad label never silently drops the existing cap. post-review treats fullsend-fix-budget/* as a pipeline-managed control label so the review agent preserves a maintainer's budget label. The label stays dormant until the reusable-fix workflow forwards PR_LABELS into the fix harness env. fix.yaml deliberately omits the PR_LABELS runner var for now: referencing an unset host var fails harness env validation (fail-closed) on every run, and the workflow does not yet set it. The consumer side is complete and activates by re-adding the runner var together with the workflow change. Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
b35ed42 to
0c6fdef
Compare
|
Thanks for the careful pass. Addressed:
|
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass at head 0c6fdef. Four findings inline: one HIGH on the bot-escalation message, and three MEDIUMs on the docs claim, the post-review test mirror, and the tighten notice. A fifth candidate (no post-fix coverage for the mirrored cap) was dropped as a duplicate of an item already raised on this PR.
|
|
||
| if [[ "${ITERATION}" -gt "${CAP}" ]]; then | ||
| if is_bot_user "${TRIGGER_SOURCE}"; then | ||
| gha_echo error "Fix iteration ${ITERATION} exceeds bot cap of ${CAP}. Escalating to human." |
There was a problem hiding this comment.
[HIGH] Bot-escalation message advertises a /fs-fix recovery path the budget label has already closed, and neither error names the label
Anchored at the bot-escalation branch; the defective sentence is line 131 (last diff line here is 129).
Verified by running scripts/pre-fix.sh at head. The budget is now applied to both caps (lines 113-114), so whenever a budget label is what triggers the escalation, CAP == HUMAN_CAP == FIX_BUDGET and ITERATION already exceeds it. The advertised recovery is therefore dead by construction.
Scenario C (bot, ITERATION=3, ITERATION_CAP=5, ITERATION_CAP_HUMAN=10, PR_LABELS=fullsend-fix-budget/2):
::error::Fix iteration 3 exceeds bot cap of 2. Escalating to human.
::error::A human can still direct the agent with /fs-fix (up to 2 total iterations).
Scenario D (human alice, same inputs) confirms the advice fails immediately:
::error::Fix iteration 3 exceeds human cap of 2.
::error::The /fs-fix loop has run 3 times. Further attempts are blocked.
This strands the maintainer because FIX_ITERATION is cumulative and derived from history, not reset by the label: fullsend-ai/fullsend .github/workflows/reusable-fix.yml:278-282 computes FIX_COMMITS from the PR's fix-authored commits then ITERATION=$((FIX_COMMITS + 1)). So applying fullsend-fix-budget/2 to a PR that already has 3 fix commits instantly blocks every subsequent manual fix command, and neither error message names the label or says to remove it. docs/fix.md does not mention removal either.
Not a duplicate of the existing threads: the outdated pre-fix.src.sh:118 thread asserted the opposite (HUMAN_CAP never tightened) and was addressed by this change, which introduced this new defect; the live agents/fix.md:191 thread covers the documentation contradiction, not the runtime message or the missing recovery instruction. scripts/pre-fix-test.sh:133 ("bot escalation reports tightened human cap", expecting "up to 2 total iterations") cements the misleading text as intended behavior.
Suggestion. Guard the bot-escalation sentence on HUMAN_CAP actually still exceeding ITERATION. When it does not, replace it with text stating the loop is fully exhausted and the fullsend-fix-budget label must be removed. Also make the human-cap error self-describing, e.g. Fix iteration N exceeds human cap of M (set by fullsend-fix-budget/M) — remove the label to continue. Update the pre-fix-test.sh:133 assertion to match, and add the removal instruction to docs/fix.md.
| |-------|---------| | ||
| | `fullsend-no-fix` | Prevents automatic fix runs on this PR. Applied by `/fs-fix-stop`. Manual `/fs-fix` commands are unaffected. | | ||
| | `needs-human` | The fix agent is approaching its iteration cap and needs human direction. Applied automatically when an automatic fix iteration reaches the warning threshold. | | ||
| | `fullsend-fix-budget/N` | Tightens the review→fix loop for this PR to `N` iterations (`N` a positive integer). Applied by a maintainer. Can only lower the applicable cap (bot or human), never raise it; malformed values are ignored. | |
There was a problem hiding this comment.
[MEDIUM] fullsend-fix-budget/N is documented as a working control although PR_LABELS is populated nowhere in the delivery path
This row states unconditionally that the label "Tightens the review→fix loop for this PR to N iterations ... Applied by a maintainer", with no caveat. But PR_LABELS is set by nothing that reaches the fix agent: harness/fix.yaml:71-78 deliberately omits it from env.runner (correctly, to avoid fail-closed validation), and grepping fullsend-ai/fullsend/.github/workflows shows PR_LABELS exists only in reusable-dispatch.yml:144 (step-scoped to "Determine stage" in a different job) — reusable-fix.yml never sets it. parse_fix_budget therefore always receives the empty string and the feature is inert.
A maintainer following these docs applies fullsend-fix-budget/2 to a risky or expensive PR, sees no error, and gets the full 5-iteration loop anyway — a silent no-op on a cost/safety knob. The PR body's "Scope note" discloses this; the user-facing docs do not.
Relatedly, the pre-existing "Iteration limits" section at docs/fix.md:149-159 still describes only the static 5/10 defaults with no mention that a PR label can lower them — that is the section an operator reads to understand behavior for a specific PR, and it will be incomplete once the wiring lands.
Distinct from the earlier review-body item that asked for this row to exist: the row has now been added, and the residual defect is that it presents the control as live with no caveat. The bot's inertness comment is anchored on scripts/pre-fix.src.sh:116 and is outdated; nothing so far raises that the user-facing docs misrepresent the control as active.
Suggestion. Mark both doc entries as not yet active (e.g. "Reserved — not yet enforced; requires PR_LABELS wiring in reusable-fix.yml") and link a tracking issue, or land the wiring in the same change. Cross-reference the label from the "Iteration limits" section once live. A gha_echo warning in pre-fix when PR_LABELS is unset would also make the dormancy visible in run logs.
| fi | ||
| # Maintainer-set fix-loop budget (fullsend-fix-budget/N); pipeline-managed so | ||
| # the review agent preserves it rather than treating it as a contextual label. | ||
| if [[ "${label}" == fullsend-fix-budget/* ]]; then |
There was a problem hiding this comment.
[MEDIUM] New control-label tests assert against a copy of is_control_label inside the test file, not the production function
post-review-test.sh defines its own inline is_control_label() at lines 311-328; it never sources post-review.src.sh (POST_SCRIPT at line 386 is only used for bash "${POST_SCRIPT}" end-to-end cases). This PR adds the fullsend-fix-budget/* branch to that duplicate here at lines 322-326, in lockstep with the two production copies (post-review.src.sh:302 and the post-review.sh bundle line 712), so the three new cases verify the test's own mirror.
Proven empirically at head: I deleted the fullsend-fix-budget/* branch from both scripts/post-review.src.sh and scripts/post-review.sh, then ran bash scripts/post-review-test.sh. Result:
PASS: fix-budget-3-is-control
PASS: fix-budget-99999-is-control
PASS: fix-budget-prefix-only-not-control
All tests passed
The tests cannot fail if the production branch is dropped or the copies drift, and this is the only coverage the post-review change gets. (The duplication predates this PR; the PR extends it.)
Suggestion. Extract is_control_label into a sourceable lib — as this PR already did for parse_fix_budget in scripts/lib/fix-budget.lib.sh — and have both post-review.src.sh and the test source it. Either way the assertion must fail when the production branch is removed.
| CAP="${HUMAN_CAP}" | ||
| fi | ||
|
|
||
| if [[ -n "${FIX_BUDGET}" && "${FIX_BUDGET}" -eq "${CAP}" ]]; then |
There was a problem hiding this comment.
[MEDIUM] Tighten notice misses the case it should report and fires on one where the governing cap did not change
The notice guard is [[ -n "${FIX_BUDGET}" && "${FIX_BUDGET}" -eq "${CAP}" ]], comparing only against the cap for the current trigger source. Since line 114 silently tightens HUMAN_CAP on every run regardless of trigger, the notice is wrong in both directions. Confirmed by running scripts/pre-fix.sh at head (ITERATION_CAP=5, ITERATION_CAP_HUMAN=10, bot trigger, iteration 1):
PR_LABELS=fullsend-fix-budget/9→ no notice at all, yet the human cap was silently cut 10→9. This is the half with teeth: a maintainer gets no signal that they reduced the human escape hatch, which is exactly the lockout surface flagged in the HIGH finding on the escalation message.PR_LABELS=fullsend-fix-budget/5→::notice::PR label fullsend-fix-budget/5 caps the fix loop at 5 iteration(s).even though the bot cap was already 5 and did not change; the notice never mentions that the human cap went 10→5.
pre-fix-test.sh covers only budget > cap (asserting no notice, lines 120-127) and budget < cap (notice, line 116). The budget == default-cap case and the human-cap-only-tightened case are both untested.
Suggestion. Track whether each cap actually decreased (e.g. set a flag inside each [[ ... -lt ... ]] branch) and emit a notice naming which caps were lowered and to what — including the human cap when only it changed. Add pre-fix-test.sh cases for budget == default bot cap and for a budget that tightens only the human cap.
2 participants
What
Adds a
fullsend-fix-budget/NPR label that lets a maintainer cap the review->fix loop for a single PR below the global iteration cap.The label can only tighten the cap, never raise it. In
pre-fix, after the bot/human cap is selected, the parsed budget is applied asmin(label_budget, cap). Afullsend-fix-budget/2label on a PR that would otherwise get the bot cap of 5 stops the loop after 2 fix cycles; afullsend-fix-budget/99label is ignored (it cannot loosen the cap).Why
Today the fix-loop ceiling is global (
ITERATION_CAP/ITERATION_CAP_HUMAN). There is no per-PR knob when a maintainer wants a specific change to burn fewer cycles before escalating to a human, for example on a risky or expensive PR. A label is the lightest touch: it lives on the PR, needs no config change, and degrades safely.How
scripts/lib/fix-budget.lib.shwithparse_fix_budget, which extracts the smallest validfullsend-fix-budget/Nfrom a newline-separatedPR_LABELS. Malformed values (non-integer, zero, negative) are ignored rather than fatal, so a bad label never silently drops the existing cap.scripts/pre-fix.src.shsources the lib and applies the tightening after the cap is chosen (emits anoticewhen it takes effect).scripts/pre-fix-test.shunit-tests the parser directly (it is pure, so no forge mocks are needed) and is registered in the Makefilescript-testblock.scripts/pre-fix.shviamake script-build;make check-bundleandmake script-testpass.Scope note
PR_LABELSis a new optional input that the fix workflow does not populate yet. Wiring it (one line passing the PR's labels into the pre-fix step's env) is a natural follow-up; until then the feature is inert and the cap behaves exactly as before. Keeping the wiring separate keeps this PR to the parsing/enforcement logic plus its test.Relationship to the retro anti-retry-budget stance
The retro-analysis skill argues against retry budgets (
skills/retro-analysis/SKILL.md), but that is about masking test flakiness by retrying flaky tests, a correctness-signal concern. This is a different axis: a ceiling on how many times the review->fix loop runs before escalating to a human. It does not retry a failing check to make it pass; it bounds autonomous iteration. The two do not conflict.